Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes + check for dupes contexts in postgres/redshift #139

Merged

Conversation

rlh1994
Copy link
Contributor

@rlh1994 rlh1994 commented Sep 12, 2023

Description & motivation

@agnessnowplow Raised an issue around when the session/user identifiers have the same context it fails on PG/RS (due to duplicate CTEs with the same name). This is actually a wider issue, it would fail with the same context used multiple times in one of the identifiers, and also if you use the context as an identifier then add it in the entity_or_sde as well. Giving each CTE a custom name felt complicated, so I have instead made the logic directly on the base macros.

Basically, I have got a distinct list of contexts that need CTEing and joining, and where there are duplicates it uses the alias and prefix of the FIRST instance, but the FIELD of the actual record (please double check I haven't misses this anywhere). I do not raise a warning in the manifest because they should never need to access the inner workings of this code so we can just manage it for them.

I also made a few other tweaks while I was here.

We should probably add some tests for this, but that felt like a large task that we may not have time to do before release

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@rlh1994 rlh1994 requested a review from emielver as a code owner September 12, 2023 12:42
@rlh1994 rlh1994 changed the base branch from main to release/snowplow-utils/0.15.0 September 12, 2023 12:43
Copy link
Contributor

@emielver emielver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rlh1994 rlh1994 force-pushed the fix/duplicate-identifier-schemas branch 2 times, most recently from 0a8f151 to 5ece8b9 Compare September 14, 2023 09:21
@rlh1994 rlh1994 force-pushed the fix/duplicate-identifier-schemas branch from 5ece8b9 to dee1baf Compare September 14, 2023 09:32
@rlh1994 rlh1994 merged commit 9ae1a21 into release/snowplow-utils/0.15.0 Sep 14, 2023
4 checks passed
@rlh1994 rlh1994 deleted the fix/duplicate-identifier-schemas branch September 14, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants